Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cleaning up LowTemperatureRadiantSystems #8569

Merged
merged 15 commits into from
Mar 1, 2021
Merged

Cleaning up LowTemperatureRadiantSystems #8569

merged 15 commits into from
Mar 1, 2021

Conversation

jmythms
Copy link
Contributor

@jmythms jmythms commented Feb 26, 2021 •
edited
Loading

Pull request overview

Focuses on LowTemperatureRadiantSystems.*

  • Adds default constructors.
  • Removed non-const Statics
  • Removed non-const externs

NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process

Reviewer

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

@jmythms jmythms added the Defect Includes code to repair a defect in EnergyPlus label Feb 26, 2021
@jmythms jmythms added this to the EnergyPlus 9.5.0 milestone Feb 26, 2021
@jmythms jmythms self-assigned this Feb 26, 2021
@jmythms jmythms marked this pull request as ready for review February 27, 2021 14:32
Myoldmopar
Myoldmopar approved these changes Mar 1, 2021
View reviewed changes
Copy link
Member

@Myoldmopar Myoldmopar left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really great, thanks @jmarrec @jmythms !

src/EnergyPlus/LowTempRadiantSystem.hh
extern Array1D_bool MySizeFlagHydr;
extern Array1D_bool MySizeFlagCFlo;
extern Array1D_bool MySizeFlagElec;
extern Array1D_bool CheckEquipName;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay, lots of externs gone!!

src/EnergyPlus/LowTempRadiantSystem.hh
Real64 todayRunningMeanOutdoorDryBulbTemperature = 0.0; // Current running mean outdoor air dry-bulb temperature
Real64 yesterdayRunningMeanOutdoorDryBulbTemperature = 0.0; // Running mean outdoor air dry-bulb temperature from yesterday
Real64 todayAverageOutdoorDryBulbTemperature = 0.0; // Average outdoor dry-bulb temperature for today
Real64 yesterdayAverageOutdoorDryBulbTemperature = 0.0; // Average outdoor dry-bulb temperature for yesterday
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

src/EnergyPlus/LowTempRadiantSystem.hh
@@ -214,60 +173,53 @@ namespace LowTempRadiantSystem {

virtual void reportLowTemperatureRadiantSystem(EnergyPlusData &state) = 0;

// Default Constructor
RadiantSystemBaseData() = default;
~RadiantSystemBaseData() = default;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

src/EnergyPlus/LowTempRadiantSystem.hh
Array1D<LowTempRadiantSystem::ElecRadSysNumericFieldData> ElecRadSysNumericFields;
Array1D<LowTempRadiantSystem::HydronicRadiantSysNumericFieldData> HydronicRadiantSysNumericFields;
Array1D<LowTempRadiantSystem::ConstantFlowRadDesignData> CflowRadiantSysDesign;
Array1D<LowTempRadiantSystem::VarFlowRadDesignData> HydronicRadiantSysDesign;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allll these were globals. Very nice!

tst/EnergyPlus/unit/LowTempRadiantSystem.unit.cc
ElecRadSys.allocate(1);
HydrRadSys.allocate(1);
CFloRadSys.allocate(1);
state->dataLowTempRadSys->ElecRadSys.allocate(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly what I expected.

src/EnergyPlus/LowTempRadiantSystem.hh
@@ -592,17 +504,100 @@ namespace LowTempRadiantSystem {
LowTempRadiantSystem::SystemType const SystemType // Type of radiant system: hydronic, constant flow, or electric
);

void UpdateRadSysSourceValAvg(bool &LowTempRadSysOn); // .TRUE. if the radiant system has run this zone time step
void UpdateRadSysSourceValAvg(EnergyPlusData &state,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I was expecting this possibility as well. Looks like just this one function needed a state argument though, so that's great!

@Myoldmopar
Copy link
Member

CI is all super green, these changes look great, merging. Thanks @jmythms

@Myoldmopar Myoldmopar merged commit 171ad02 into develop Mar 1, 2021
@Myoldmopar Myoldmopar deleted the LowTempRadOptimize branch March 1, 2021 18:01
@jmythms
Copy link
Contributor Author

jmythms commented Mar 1, 2021

CI is all super green, these changes look great, merging. Thanks @jmythms

Thank you!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Myoldmopar Myoldmopar Myoldmopar approved these changes

Assignees

@jmythms jmythms

Labels
Defect Includes code to repair a defect in EnergyPlus
Projects
None yet
Milestone
EnergyPlus 9.5.0
Development

Successfully merging this pull request may close these issues.
6 participants
@jmythms @Myoldmopar @nrel-bot @nrel-bot-3 @nrel-bot-2b @nrel-bot-2c